Fix: detect L3 worker child exit instead of spinning forever by ndleslx · Pull Request #1003 · hw-native-sys/simpler

ndleslx · 2026-06-08T03:18:30Z

Summary

Plumb the forked child pid from the Python Worker through the nanobind bindings into LocalMailboxEndpoint
Sample child liveness with waitpid(WNOHANG) while spin-polling for TASK_DONE / CONTROL_DONE, so a child that exits early is reported instead of spun on forever
Report a dead child as an ENDPOINT_FAILURE completion on the task path; throw and poison the mailbox on the control path

Motivation

Fixes the L3 Worker hang from #980: a chip child could exit before publishing TASK_DONE, leaving the parent polling the mailbox indefinitely and the child <defunct>.

The hang is still present on main — LocalMailboxEndpoint::run spin-polls TASK_DONE with no liveness check and no timeout, and worker_manager.cpp contains no waitpid at all.

Changes since the original commit

Rebased onto current main (the branch was ~295 commits behind) and adapted to the WorkerThread → WorkerEndpoint / LocalMailboxEndpoint refactor, so the check now lives in the endpoint that actually owns the mailbox and both spin loops.

Review feedback addressed in check_child_death():

retry waitpid on EINTR instead of treating it as a fatal error
treat any other waitpid failure as "no information" and keep polling, rather than tearing down a live worker
only record the child as dead once it was actually reaped or observed unwaitable, and retain the exit status so every later operation reports the same cause instead of silently resuming the spin

Two adaptations to current main:

the task path returns an ENDPOINT_FAILURE completion (matching the current error model) rather than throwing
the control loop does not sleep between iterations on main, so it samples on a 10 ms wall-clock period; the task loop keeps the count-based interval (200 iterations × 50 us ≈ 10 ms)

The mailbox is deliberately not reset to IDLE before throwing: with the child gone, no later command can complete, so admitting one would restore the hang this check exists to break. The existing mailbox_control_timed_out_ poison flag is reused instead.

The original teardown change is dropped — every os.waitpid site in Worker close on current main already tolerates ChildProcessError.

Testing

New regression test WorkerManagerTest.ControlCommandFailsWhenChildExitsBeforeCompletion: forks a real child that exits without publishing CONTROL_DONE. Verified it fails without the fix (hangs into the 10 s guard) and passes with it (10 ms).
ctest C++ unit suite: 60/60 passed
pytest tests/ut/py/test_worker tests/ut/py/test_chip_worker.py: 348 passed, 2 skipped
L3 scene tests onboard (task-submit, a2a3): test_l3_dependency, test_l3_group, test_l3_host_buffer_registration passed
examples/workers/l3/child_memory/main.py -p a2a3 onboard: golden + cross-invocation checks passed
pre-commit (clang-format, clang-tidy, cpplint, ruff, pyright): all passed

Fixes #980

coderabbitai · 2026-06-08T03:18:37Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a039dde4-b9b5-4a7b-b184-b83b17d7fbcd

📥 Commits

Reviewing files that changed from the base of the PR and between 0ebff86 and ed8e3b3.

📒 Files selected for processing (7)

python/bindings/worker_bind.h
python/simpler/worker.py
src/common/hierarchical/worker.cpp
src/common/hierarchical/worker.h
src/common/hierarchical/worker_manager.cpp
src/common/hierarchical/worker_manager.h
tests/ut/cpp/hierarchical/test_scheduler.cpp

📝 Walkthrough

Walkthrough

Child PIDs now flow through hierarchical worker registration into local mailbox endpoints. Endpoints periodically inspect child status during task and control waits, report exit details, poison failed control mailboxes, and are covered by a fork-based regression test.

Changes

Child liveness detection

Layer / File(s)	Summary
Registration contracts and startup wiring `python/bindings/worker_bind.h`, `python/simpler/worker.py`, `src/common/hierarchical/worker.h`, `src/common/hierarchical/worker.cpp`, `src/common/hierarchical/worker_manager.h`, `src/common/hierarchical/worker_manager.cpp`	Worker registration accepts optional child PIDs, validates mailbox/PID list lengths, and passes PIDs into local mailbox endpoints.
Mailbox child-death handling `src/common/hierarchical/worker_manager.h`, `src/common/hierarchical/worker_manager.cpp`	Endpoints use `waitpid(WNOHANG)` during task and control-command polling, report exit reasons, and poison control mailboxes after child death.
Premature-exit regression coverage `tests/ut/cpp/hierarchical/test_scheduler.cpp`	A fork-based test verifies bounded failure reporting and immediate failure of subsequent control operations after child termination.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Worker
  participant WorkerManager
  participant LocalMailboxEndpoint
  participant ChildProcess
  Worker->>WorkerManager: register mailbox and child_pid
  WorkerManager->>LocalMailboxEndpoint: construct endpoint with child_pid
  LocalMailboxEndpoint->>ChildProcess: waitpid(WNOHANG)
  ChildProcess-->>LocalMailboxEndpoint: exit status
  LocalMailboxEndpoint-->>WorkerManager: report endpoint failure
  WorkerManager-->>Worker: return child-death error

Possibly related PRs

hw-native-sys/simpler#1381: Both modify the hierarchical startup path involving _start_hierarchical().
hw-native-sys/simpler#1396: Both add child-process liveness handling during hierarchical worker startup and mailbox operation.

Poem

A bunny watched the child process hop,
And caught its exit before the loop could stop.
PIDs now travel, mailboxes know,
Dead workers leave a reason to show.
No endless spin beneath the moon—
The test says failure comes soon!

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

gemini-code-assist

Code Review

This pull request introduces child process PID tracking and liveness checks to prevent the parent process from spin-polling indefinitely if a child worker dies. The review feedback highlights several critical improvements: replacing the Python 3.9-incompatible strict=True in zip() with manual length checks, improving the robustness of check_child_alive in C++ (such as handling EINTR and correcting child_reaped_ logic), and resetting the mailbox state to IDLE when exceptions are thrown during polling to prevent deadlocks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Hierarchical Worker startup could hang forever. A chip (L2) child that failed ChipWorker.init only wrote its error and exited, and the parent spun on `while state != INIT_DONE` with no timeout and no liveness check, so a dead or slow child deadlocked startup (the hw-native-sys#1003 / hw-native-sys#980 class of hang). Next-level (L4->L3) children had no readiness barrier at all: a failing inner init unwound back through the forked copy of the caller's frames instead of reporting a bounded error. Make startup a strong READY boundary: - States: INIT_READY / INIT_FAILED (renamed INIT_DONE), mirrored in the C++ MailboxState enum for parity. - Every forked child (sub, chip, next-level) publishes INIT_READY after its own init succeeds, or INIT_FAILED carrying the original error. A shared `_forked_child_main` guarantees a child always terminates via os._exit and never unwinds an exception into the parent's startup frames (which would let it SIGKILL its inherited-PID siblings); READY is published only after all fallible setup (inner init AND identity tables). - Parent barrier `_await_children_ready` waits per child with a deadline (`startup_timeout_s`, default 300s, validated positive+finite) and a waitpid(WNOHANG) liveness check; INIT_FAILED, a dead child, or a blown deadline raises a bounded RuntimeError. Applied to the sub, chip, and next-level edges (chip and next-level had no/weak barriers before). - Rollback (`_abort_hierarchical`, run on any pre-scheduler BaseException, including KeyboardInterrupt): next-level children that reached their serve loop are closed gracefully (SHUTDOWN + bounded wait) so they unlink their own nested shms; the rest are SIGKILLed; PIDs the barrier already reaped are excluded from the kill (no reused-PID signal); all are reaped and the mailboxes freed. A sibling still mid-init when another fails is SIGKILLed and its nested shms are reclaimed by the resource_tracker at exit — closing that race needs the eager-init handshake (follow-up). Next-level children are still started lazily on first run(); the barrier is recursive plumbing so a later eager-init change makes INIT_READY propagate up only after the whole subtree is ready, with no protocol change. Adds device-free ut coverage (next-level + sub init injection, faked ChipWorker on a2a3sim): bounded init-failure error, child-exit-before- ready, deadline-fires, rollback-reaps-no-leak, graceful-close of a READY sibling, sub-edge liveness, timeout config validation, and healthy-tree happy paths. Every failure test is wrapped in a hard SIGALRM budget so a regression fails fast instead of hanging CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Hierarchical Worker startup could hang forever. A chip (L2) child that failed ChipWorker.init only wrote its error and exited, and the parent spun on `while state != INIT_DONE` with no timeout and no liveness check, so a dead or slow child deadlocked startup (the #1003 / #980 class of hang). Next-level (L4->L3) children had no readiness barrier at all: a failing inner init unwound back through the forked copy of the caller's frames instead of reporting a bounded error. Make startup a strong READY boundary: - States: INIT_READY / INIT_FAILED (renamed INIT_DONE), mirrored in the C++ MailboxState enum for parity. - Every forked child (sub, chip, next-level) publishes INIT_READY after its own init succeeds, or INIT_FAILED carrying the original error. A shared `_forked_child_main` guarantees a child always terminates via os._exit and never unwinds an exception into the parent's startup frames (which would let it SIGKILL its inherited-PID siblings); READY is published only after all fallible setup (inner init AND identity tables). - Parent barrier `_await_children_ready` waits per child with a deadline (`startup_timeout_s`, default 300s, validated positive+finite) and a waitpid(WNOHANG) liveness check; INIT_FAILED, a dead child, or a blown deadline raises a bounded RuntimeError. Applied to the sub, chip, and next-level edges (chip and next-level had no/weak barriers before). - Rollback (`_abort_hierarchical`, run on any pre-scheduler BaseException, including KeyboardInterrupt): next-level children that reached their serve loop are closed gracefully (SHUTDOWN + bounded wait) so they unlink their own nested shms; the rest are SIGKILLed; PIDs the barrier already reaped are excluded from the kill (no reused-PID signal); all are reaped and the mailboxes freed. A sibling still mid-init when another fails is SIGKILLed and its nested shms are reclaimed by the resource_tracker at exit — closing that race needs the eager-init handshake (follow-up). Next-level children are still started lazily on first run(); the barrier is recursive plumbing so a later eager-init change makes INIT_READY propagate up only after the whole subtree is ready, with no protocol change. Adds device-free ut coverage (next-level + sub init injection, faked ChipWorker on a2a3sim): bounded init-failure error, child-exit-before- ready, deadline-fires, rollback-reaps-no-leak, graceful-close of a READY sibling, sub-edge liveness, timeout config validation, and healthy-tree happy paths. Every failure test is wrapped in a hard SIGALRM budget so a regression fails fast instead of hanging CI.

A chip or sub child that exits before publishing TASK_DONE / CONTROL_DONE left the parent spin-polling its mailbox indefinitely, with the child stuck as <defunct> (issue hw-native-sys#980). Neither spin loop had any liveness check, so the only exit was the control path's timeout — and the task dispatch path had no timeout at all. Plumb the forked child pid from the Python Worker through the nanobind bindings into LocalMailboxEndpoint, and sample the child's liveness with waitpid(WNOHANG) while waiting for mailbox completion. A dispatch whose child died is reported as an ENDPOINT_FAILURE completion; a control command throws and poisons the mailbox, since no later command on a dead child could complete either. The liveness probe retries on EINTR, treats any other waitpid failure as "no information" and keeps polling, and only records the child as dead once it was actually reaped or observed unwaitable. The exit status is retained so every later operation on that endpoint reports the same cause rather than silently resuming the spin. Based on the fix by ndleslx in hw-native-sys#1003, ported onto the WorkerEndpoint refactor. The teardown half of that change is no longer needed: every os.waitpid site in Worker close already tolerates ChildProcessError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ChaoWao · 2026-07-25T04:43:17Z

Heads-up: I force-pushed this branch, replacing 109b63ec with a rebased version (your original commit is still reachable via the force-push record above and your local reflog). Happy to revert if you'd rather drive this yourself — the work is also on ChaoWao:pr-1003-rebase.

Why I picked it up: the hang is still live on main. LocalMailboxEndpoint::run spin-polls TASK_DONE with no liveness check and no timeout, and there is no waitpid anywhere in worker_manager.cpp — so #980 was closed without the code fix landing. Your diagnosis and fix shape were right.

What changed:

Rebased onto current main (~295 commits). WorkerThread was refactored into WorkerEndpoint / LocalMailboxEndpoint, so the check now lives in the class that owns the mailbox and both spin loops. There's also a third binding (add_next_level_worker_at) on main that needed the same pid parameter.
Addressed the review feedback on check_child_alive — this was the one substantive correctness gap. EINTR now retries instead of being reported as a fatal waitpid failure; any other failure is treated as "no information" and polling continues rather than killing a live worker; and the child is only marked dead once actually reaped, with the exit status retained so later operations report the same cause instead of silently falling back into the infinite spin (the old unconditional child_reaped_ = true meant the death was reported exactly once).
Task path now returns an ENDPOINT_FAILURE completion rather than throwing, matching the error model main adopted.
Control loop samples on a 10 ms wall clock — it no longer sleeps between iterations on main, so a count-based interval would have hammered waitpid.
Dropped the teardown half (_waitpid_if_child): every os.waitpid site in Worker close on main already catches ChildProcessError.

On two review suggestions I deliberately did not apply: resetting the mailbox to IDLE before throwing would let a later command be admitted against a dead child and hang again, so I reused the existing mailbox_control_timed_out_ poison flag instead; and the zip(strict=True) note was already stale — that code used explicit length checks, which I kept as a named helper.

Added WorkerManagerTest.ControlCommandFailsWhenChildExitsBeforeCompletion, which forks a real child that exits without publishing CONTROL_DONE. I confirmed it fails without the fix (hangs into its 10 s guard) and passes with it. Full results in the PR body — C++ 60/60, Python 348 passed, plus the L3 scene tests and your child_memory repro onboard on a2a3.

Still marked draft; flip it to ready when you're happy with it.

A chip or sub child that exits before publishing TASK_DONE / CONTROL_DONE left the parent spin-polling its mailbox indefinitely, with the child stuck as <defunct> (issue hw-native-sys#980). Neither spin loop had any liveness check, so the only exit was the control path's timeout — and the task dispatch path had no timeout at all. Plumb the forked child pid from the Python Worker through the nanobind bindings into LocalMailboxEndpoint, and sample the child's liveness with waitpid(WNOHANG) while waiting for mailbox completion. A dispatch whose child died is reported as an ENDPOINT_FAILURE completion; a control command throws and poisons the mailbox, since no later command on a dead child could complete either. The liveness probe retries on EINTR, treats any other waitpid failure as "no information" and keeps polling, and only records the child as dead once it was actually reaped or observed unwaitable. The exit status is retained so every later operation on that endpoint reports the same cause rather than silently resuming the spin. Based on the fix by ndleslx in hw-native-sys#1003, ported onto the WorkerEndpoint refactor. The teardown half of that change is no longer needed: every os.waitpid site in Worker close already tolerates ChildProcessError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ChaoWao · 2026-07-25T05:15:05Z

Rebased again onto current main — #1459 ("Refactor: isolate hierarchical worker run state") landed mid-review and dropped the this argument from WorkerThread::start, which conflicted with the endpoint construction sites. Resolved by taking main's new signature plus the child_pid argument. The PR was briefly showing a merge conflict; it is MERGEABLE again now.

CI on ed8e3b3a: 14 of 16 green, including st-onboard-a2a3 and ut-a2a3 — the hardware jobs that actually exercise this path.

The two red checks are st-onboard-a5 and ut-a5, both failing in the Checkout repository step (18 s / 28 s, before any build or test runs). Not caused by this change — the same two jobs fail at the same step on every other recent PR:

scene-test-golden-hooks (run 30143296491) — st-onboard-a5=failure ut-a5=failure
codex/worker-async-pr1-run-fence (run 30142877917) — same two, same step
blockdim-auto (run 30142056115) — same two, same step

The a5 self-hosted runner looks to be in a bad state repo-wide; worth a maintainer look independently of this PR. Every a5 job that does not need that runner (st-sim-a5 on both ubuntu and macos) passes here.

gemini-code-assist Bot reviewed Jun 8, 2026

View reviewed changes

Comment thread python/simpler/worker.py Outdated

Comment thread src/common/hierarchical/worker_manager.cpp Outdated

Comment thread src/common/hierarchical/worker_manager.cpp

Comment thread src/common/hierarchical/worker_manager.cpp Outdated

ndleslx force-pushed the fix/l3-worker-child-exit-detection branch 2 times, most recently from 7aa5071 to 109b63e Compare June 8, 2026 06:35

ChaoWao mentioned this pull request Jul 18, 2026

fix(worker): bound hierarchical startup with readiness protocol #1396

Merged

4 tasks

ChaoWao mentioned this pull request Jul 19, 2026

fix(worker): unify lifecycle into one authoritative, terminal-close() machine #1398

Merged

3 tasks

ChaoWao force-pushed the fix/l3-worker-child-exit-detection branch from 109b63e to 7616dd6 Compare July 25, 2026 04:36

ChaoWao changed the title ~~Fix L3 worker child-exit hangs~~ Fix: detect L3 worker child exit instead of spinning forever Jul 25, 2026

ChaoWao force-pushed the fix/l3-worker-child-exit-detection branch from 7616dd6 to ed8e3b3 Compare July 25, 2026 04:56

ChaoWao marked this pull request as ready for review July 25, 2026 07:03

ChaoWao approved these changes Jul 25, 2026

View reviewed changes

ChaoWao merged commit bdf6fed into hw-native-sys:main Jul 25, 2026
13 of 16 checks passed

coderabbitai Bot mentioned this pull request Jul 26, 2026

Fix: exit a forked worker child once its parent is gone #1495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: detect L3 worker child exit instead of spinning forever - #1003

Fix: detect L3 worker child exit instead of spinning forever#1003
ChaoWao merged 1 commit into
hw-native-sys:mainfrom
ndleslx:fix/l3-worker-child-exit-detection

ndleslx commented Jun 8, 2026 •

edited by ChaoWao

Loading

Uh oh!

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChaoWao commented Jul 25, 2026

Uh oh!

ChaoWao commented Jul 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ndleslx commented Jun 8, 2026 • edited by ChaoWao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Changes since the original commit

Testing

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChaoWao commented Jul 25, 2026

Uh oh!

ChaoWao commented Jul 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ndleslx commented Jun 8, 2026 •

edited by ChaoWao

Loading

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading